x/bank: use internal package #4521

alessio · 2019-06-09T06:00:30Z

Reorganise x/bank packages and leverage internal special
package for enhanced encapsulation.

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: clog add [section] [stanza] [message]
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Reorganise x/bank sub-packages and leverage internal special package for enhanced encapsulation.

codecov · 2019-06-09T06:05:57Z

Codecov Report

Merging #4521 into master will decrease coverage by 0.4%.
The diff coverage is 23.52%.

@@            Coverage Diff             @@
##           master    #4521      +/-   ##
==========================================
- Coverage   53.49%   53.09%   -0.41%     
==========================================
  Files         256      257       +1     
  Lines       16165    16285     +120     
==========================================
- Hits         8648     8646       -2     
- Misses       6870     6992     +122     
  Partials      647      647

fedekunze · 2019-06-09T08:06:56Z

x/bank/internal/handler/handler.go

@@ -1,15 +1,16 @@
-package bank
+package handler


hmm shouldn't this be under internal ?

Nope, it needs to be handler. I could move it under types

Nope, it's needs to be handler.

is it because a restriction from internal that it can't live underx/bank/internal/handler.go ? I'm fine with it as long as we update #4438

I'm unsure about using internal just like other normal packages, i.e. dropping .go files in internal may not be permitted as internal may not compile.

Hmmm, don't we want files like abci.go and handler.go to live in the parent/root of the module and not under internal? I.e. I wouldn't like to see a module turn into everything being shoved into internal with just an alias.go at the root.

x/bank/internal/handler/handler_test.go

fedekunze · 2019-06-09T08:08:00Z

x/bank/internal/invariants/invariants.go

@@ -1,12 +1,12 @@
-package bank
+package invariants


shouldn't this be under keeper as other modules have it?

I'm fine with that

import cycles - I'd keep this as it is

why? it shouldn't have any

Possibly - will double check

But I don't think that that is the reason - bank's internal types depend on each other

The scope of this PR is not to introduce functional changes/refactoring. So please as long as it works just +1 :)

I still think this should leave in keeper

Is this possible @alessio? I don't mind helping out here!

x/bank/internal/tags/tags.go

x/bank/internal/keeper/app_test.go

…cular deps

x/bank/internal/invariants/msgs.go

x/bank/internal/types/tags.go

alexanderbez

Mostly LGTM -- thanks @alessio. I don't think all the choices made here though are fully agreed upon. Namely, simulation messages and what should live under the root of the module. I don't want to see us starting to shove everything under internal/.

alexanderbez · 2019-06-11T03:41:13Z

x/bank/internal/handler/handler.go

@@ -1,15 +1,16 @@
-package bank
+package handler


Hmmm, don't we want files like abci.go and handler.go to live in the parent/root of the module and not under internal? I.e. I wouldn't like to see a module turn into everything being shoved into internal with just an alias.go at the root.

x/bank/internal/invariants/msgs.go

alessio · 2019-06-11T06:28:05Z

Comments were addressed

x/bank/internal/invariants/invariants.go

x/bank/alias.go

Co-Authored-By: Karoly Albert Szabo <szabo.karoly.a@gmail.com>

alexanderbez · 2019-06-11T22:04:24Z

Closing this issue for now @alessio. Let's discuss further as most believe that this may not be the best approach after all. May potentially reopen after 👍

alessio · 2019-06-13T12:40:26Z

Reopening this

alessio · 2019-06-13T12:42:51Z

Promote handler.go to top-level package
Promote simulation msgs to top-level

fedekunze

few structural changes required + clog entry

fedekunze · 2019-06-14T12:22:08Z

x/bank/internal/invariants/invariants.go

@@ -1,12 +1,12 @@
-package bank
+package invariants


I still think this should leave in keeper

x/bank/module.go

fedekunze · 2019-06-14T12:23:00Z

x/bank/module.go

@@ -90,7 +91,7 @@ func (AppModule) Name() string {

 // register invariants
 func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) {
-	RegisterInvariants(ir, am.accountKeeper)
+	invariants.RegisterInvariants(ir, am.accountKeeper)


fedekunze · 2019-06-14T12:23:25Z

x/bank/msgs.go

@@ -1,4 +1,4 @@
-package simulation


this should remain in the simulation package

alexanderbez

Just a few minor nitpicks, but otherwise LGTM 👍

alexanderbez · 2019-06-14T12:34:35Z

x/bank/app_test.go

@@ -1,11 +1,13 @@
-package bank
+package bank_test


Can we rename this file abci_test.go and the corresponding file to abci.go?

this file contains a lot of mixed tests (some from keepers and others from msgs). I'd consider addressing this in a follow PR

x/bank/alias.go

alexanderbez · 2019-06-14T12:41:33Z

x/bank/msgs.go

@@ -1,4 +1,4 @@
-package simulation
+package bank


Can we keep this structure as-is/unchanged (i.e. x/bank/simulation/*)? I believe this is the pattern we want to follow in modules.

I thought we said we wanted to keep only client as top-level package?

Simulation messages live top-level already, isn't that OK?

alexanderbez · 2019-06-14T12:42:58Z

x/bank/internal/invariants/invariants.go

@@ -1,12 +1,12 @@
-package bank
+package invariants


Is this possible @alessio? I don't mind helping out here!

fedekunze

LGTM

alexanderbez

ACK

Reorganise x/bank packages and leverage internal special package for enhanced encapsulation.

alessio changed the title ~~x/bank: use internal pacakge~~ x/bank: use internal package Jun 9, 2019

x/bank: use internal package

9e07d69

Reorganise x/bank sub-packages and leverage internal special package for enhanced encapsulation.

alessio force-pushed the alessio/bank-internal branch from 71729c6 to 9e07d69 Compare June 9, 2019 06:05

alessio marked this pull request as ready for review June 9, 2019 06:07

alessio requested review from alexanderbez, jackzampolin and rigelrozanski as code owners June 9, 2019 06:07

alessio requested review from fedekunze and sabau June 9, 2019 06:07

fedekunze reviewed Jun 9, 2019

View reviewed changes

alessio added 3 commits June 9, 2019 10:58

Reduce no of exported symbols

6c44e56

Merge simulation/invariant, keep handler/keeper separate to avoid cir…

75ab6ec

…cular deps

ACK fede's suggestion

8139158

alessio force-pushed the alessio/bank-internal branch from a2976ca to 8139158 Compare June 9, 2019 11:20

fedekunze reviewed Jun 9, 2019

View reviewed changes

x/bank/internal/invariants/msgs.go Outdated Show resolved Hide resolved

x/bank/internal/types/tags.go Show resolved Hide resolved

alessio requested a review from fedekunze June 10, 2019 17:20

alexanderbez added Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. T: Dev UX UX for SDK developers (i.e. how to call our code) ready-for-review labels Jun 11, 2019

alexanderbez suggested changes Jun 11, 2019

View reviewed changes

ACK bez comments

426f130

alessio requested a review from alexanderbez June 11, 2019 06:27

alessio added 2 commits June 11, 2019 08:56

Hide more internals

d2bd2c6

Hide more stuff

07725d0

sabau reviewed Jun 11, 2019

View reviewed changes

x/bank/internal/invariants/invariants.go Outdated Show resolved Hide resolved

sabau reviewed Jun 11, 2019

View reviewed changes

x/bank/alias.go Show resolved Hide resolved

Update x/bank/internal/invariants/invariants.go

9cf7dd3

Co-Authored-By: Karoly Albert Szabo <szabo.karoly.a@gmail.com>

alexanderbez closed this Jun 11, 2019

alessio reopened this Jun 13, 2019

fedekunze suggested changes Jun 14, 2019

View reviewed changes

alexanderbez suggested changes Jun 14, 2019

View reviewed changes

alessio added 3 commits June 14, 2019 15:44

ACK fede suggestions

1c723d6

Add pending entry

e9f5d97

Rename msgs.go to simulation.go

1e2db7b

alessio requested a review from fedekunze June 14, 2019 13:54

fedekunze approved these changes Jun 14, 2019

View reviewed changes

alexanderbez approved these changes Jun 14, 2019

View reviewed changes

alessio merged commit 1e9ca4a into master Jun 14, 2019

alessio deleted the alessio/bank-internal branch June 14, 2019 14:10

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

x/bank: use internal package (cosmos#4521)

2f639dc

Reorganise x/bank packages and leverage internal special package for enhanced encapsulation.

x/bank: use internal package #4521

x/bank: use internal package #4521

Conversation

alessio commented Jun 9, 2019

codecov bot commented Jun 9, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

alessio Jun 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio commented Jun 11, 2019

alexanderbez commented Jun 11, 2019

alessio commented Jun 13, 2019

alessio commented Jun 13, 2019 • edited Loading

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2019 •

edited

Loading

alessio Jun 9, 2019 •

edited

Loading

alessio commented Jun 13, 2019 •

edited

Loading